Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix exception handling on Python 3.12 #3306

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jul 10, 2023

Reworked exception handling for Python 3.12. Ref #3305

The simplifications to PyErrState result in a ~10-15% perf improvement in the bench_err benchmarks on all Python versions. On 3.12 the enum size has been reduced from 32 to 24 bytes, which might have small global perf benefits too.

While working on this I noticed a bug on <3.12 where the original panic message is not retrieved properly in PyErr::fetch if the PanicException is normalised. I've added that as a second commit.

src/err/mod.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Pushed a more complete implementation which I'm happy with, and updated OP.

src/err/err_state.rs Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
src/exceptions.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

I think best if I circle back to this later to split out into multiple PRs for the changes of behaviour / fixes etc.

src/err/err_state.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Assuming CI is green, I believe this is now good to review.

@davidhewitt
Copy link
Member Author

On further reflection, I should be able to split this out to first make the PyErrState::LazyValue & PyErrState::LazyTypeAndValue -> PyErrState::Lazy change a separate PR, if that would help with reviewing.

@adamreichold
Copy link
Member

if that would help with reviewing.

I am pretty swamped at the moment, so anything that turns this into smaller chunks would be appreciated.

@davidhewitt
Copy link
Member Author

if that would help with reviewing.

I am pretty swamped at the moment, so anything that turns this into smaller chunks would be appreciated.

👍 #3323

@davidhewitt
Copy link
Member Author

Ok, I think this is as small a diff as it can possibly get. I split off the second commit into #3326.

src/err/mod.rs Outdated Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
src/err/mod.rs Outdated Show resolved Hide resolved
@adamreichold adamreichold added this pull request to the merge queue Jul 20, 2023
Merged via the queue into PyO3:main with commit 9e87aac Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants